Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix table re-rendering by showing/hiding actions column via CSS instead #665

Merged
merged 6 commits into from
Apr 13, 2018

Conversation

jen-huang
Copy link
Contributor

Fixes #441

This PR fixes unnecessary re-rendering of the entire EuiBasicTable component upon hovering over table rows by:

  • removing hoverRow state on EuiBasicTable
  • adding the class .euiTableCellContent--showOnHover to EuiTableRowCell when prop showOnHover is true

The .euiTableCellContent--showOnHover class has styling rules to hide its content by default and show it when its parent .euiTableRow is hovered.

Normal/keyboard navigation for single and multiple action column remains the same (no functionality changes).

@jen-huang jen-huang requested review from cjcenizal and snide April 12, 2018 06:44
Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM but I haven't tested in browser. I'm going to add @nreese as a reviewer to verify this fixes his performance issue.

@cjcenizal cjcenizal requested a review from nreese April 12, 2018 14:03
@cchaos cchaos requested review from cchaos and removed request for snide April 12, 2018 16:45
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work for me, just one comment about the CSS.

@@ -144,3 +144,14 @@
overflow: visible; /* 1 */
}
}

.euiTableCellContent--showOnHover {
> * {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes way more sense to handle the hover opacities via css. But, universal selectors are not great in practice and we're trying to stay away from them in EUI as it can have unintended behaviors if the DOM is not exactly as the CSS is expecting.

To remedy this, I think you need to pass the showOnHover prop down to the child element of the EuiTableRowCell and apply the opacities directly to this. This will ensure you're targeting the correct element to show/hide.

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Thanks for fixing this.

@jen-huang
Copy link
Contributor Author

@cchaos Thanks for the feedback - updated PR to replace the wildcard selector with .euiTableCellContent__hoverItem.

@@ -146,7 +146,7 @@
}

.euiTableCellContent--showOnHover {
> * {
.euiTableCellContent__hoverItem {
opacity: 0;

.euiTableRow:hover > .euiTableRowCell > &,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The specificity of this selector is too high. Remove the > .euiTableRowCell > and it should still work with just .euiTableRow:hover &

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated!

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Jen!

@jen-huang jen-huang merged commit 89b664c into elastic:master Apr 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EuiInMemoryTable - re-rendering all table rows on row mouseover
4 participants